review: budget and shed tuning from the 07-10 production sheds#258
review: budget and shed tuning from the 07-10 production sheds#258jwbron wants to merge 1 commit into
Conversation
…-10 production sheds The 07-10 reviews on #232/#238 shed claim validation and three whole-change dimensions on substantial PRs. Recalibrate the tier budgets (overhead-aware wall clocks; low/medium invocation caps that fit the seven-reviewer roster), exempt pipeline steps from the invocation cap, gate shedding the claim-validator on hard-ceiling pressure only, and split the skipped-dimension note wording so a planned budget shed no longer reads as an infrastructure failure. This repo's ROUTING re-raises .claude/skills/** and eval plan docs from the broad trivial-docs rule.
🦋 Changeset detectedLatest commit: 0afc12e The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
khan-actions-bot
requested review from
a team,
jeresig and
kevinb-khan
and removed request for
a team
Review Guidancegithub-actions (3 files)
Common patterns2 files: The pipeline-steps-don't-consume-a-cap-slot carve-out ( Excluded from review (1 file)Not individually reviewed — generated, formatting-only, or fully explained by a common pattern above:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| tier: "low", | ||
| floored: false, | ||
| maxReviewerInvocations: 4, | ||
| maxReviewerInvocations: 8, |
There was a problem hiding this comment.
suggestion (non-blocking): The roster-fitting caps are the point of this change, yet the budget tests (router.test.ts) only assert monotonicity and tier identity — nothing ties maxReviewerInvocations to the roster it must accommodate. If a sixth entry is later added to ENABLEABLE_REVIEWERS (roster → 8) without bumping this cap, low-tier runs silently shed a dimension again — the exact regression this PR fixes — and the suite stays green. Consider locking the relationship down:
const rosterSize = 2 + ENABLEABLE_REVIEWERS.length;
expect(DEFAULT_TIER_BUDGETS.low.maxReviewerInvocations).toBeGreaterThanOrEqual(rosterSize);
expect(DEFAULT_TIER_BUDGETS.medium.maxReviewerInvocations).toBeGreaterThanOrEqual(rosterSize);| tier: "low", | ||
| floored: false, | ||
| maxReviewerInvocations: 4, | ||
| maxReviewerInvocations: 8, |
There was a problem hiding this comment.
thought (non-blocking): The 07-10 root cause was misrouting, which this PR's ROUTING re-raises already address. Raising the low cap 4 → 8 additionally decides that a correctly-routed low-risk PR should run the full seven-reviewer roster (low 8 vs medium 10 nearly collapses the two tiers on this field). That may well be intended, but it is a broader call than the two incidents evidence — worth a line in the calibration comment if "only trivial sheds by design" is the goal.
| maxToolCallsPerFinding: 3, | ||
| maxTotalToolCalls: 20, | ||
| maxWallClockMinutes: 6, | ||
| maxWallClockMinutes: 10, |
There was a problem hiding this comment.
note (non-blocking): The PR description explains maxUsd is deliberately left uncalibrated (deferred to #249). Since this calibration comment is where the next tuner will look, consider mirroring that one line here too — otherwise the table reads as fully "recalibrated against measured runs" with one field silently untouched.
| # tier. (The 2026-07-10 reviews on #232/#238 routed both as trivial docs and | ||
| # shed review dimensions for it.) | ||
| .claude/skills/** tier=medium | ||
| workflows/review/eval/**/*.md tier=low |
There was a problem hiding this comment.
nitpick (non-blocking): This pattern is wider than the tier= alignment column the rest of the file keeps, so tier=low lands one space off the grid. Accept it as the intentional outlier, or widen the column across all rules.
Low-confidence (1)
workflows/review/lib/router.ts:286— Baking the measured ~3 min overhead into every tier's wall clock is fragile; measuring elapsed time from triage-end (a second timestamp) instead would keep the table overhead-invariant.
1 participant
Summary:
The 2026-07-10 reviewer runs on #238 (review) and #232 (review) shed claim validation and three whole-change dimensions on substantial PRs, and reported the sheds with wording that reads as infrastructure failure. Root causes, from the run logs: both PRs misrouted to the bottom tiers via the broad
**/*.md tier=trivialrule; the tier wall clocks ignore the ~3 minutes of fixed staging/router/triage overhead, so a low-tier run was at 80% of its soft budget the moment its reviewers returned; the low-tier invocation cap of 4 cannot fit the seven-reviewer whole-change roster this repo (and webapp) enables, so every low-tier run shed dimensions by arithmetic; and the two runs counted the cap inconsistently (#238 chargedpattern-triageagainst it, #232 did not).Four changes:
.claude/skills/**(medium) andworkflows/review/eval/**/*.md(low) from the trivial-docs rule; both are executable-not-docs prose.router.ts): overhead-aware wall clocks (trivial 3->6, low 6->10, medium 12->15) and roster-fitting invocation caps (low 4->8, medium 8->10). Trivial stays deliberately small.maxUsdis untouched: those numbers are also unrealistic (measured runs cost $3.2-3.6 against a $0.5-1.5 target), but spend is unobservable mid-run today and review: clamp the run budget to the effective credit cap; land before the cap #249 is building the credits-cap machinery that makes it observable, so recalibrating dollars belongs there.review.md): the invocation cap counts finding-producers only;pattern-triage,thread-reconciler, andclaim-validatorare pipeline steps and never consume a slot.review.md): the claim-validator may be shed only under genuine hard-ceiling pressure (three-quarters oftimeout-minutes, or a direct credits-cap signal), never at a mere soft-target breach; its cost tracks the visible candidate count, not the diff. Skipped-dimension notes now distinguishshed under the <tier>-tier run budgetfrom<sub-agent> output unavailable, so budget arithmetic no longer masquerades as failure.Interaction with #249: complementary. #249 clamps the selected tier budget to the effective credit cap; this PR resizes the un-clamped table and the shed discipline. Expect a textual merge conflict in the review.md graceful-landing section, whichever lands second.
Test plan:
pnpm test(656 tests, all passing; the budget tests assert monotonicity and tier identity, both preserved).pnpm exec prettier --checkon the touched TS.globToRegExp(**/collapses to zero-or-more segments, soworkflows/review/eval/**/*.mdmatcheslive-ab-plan.mdat the eval root).